Fix lazy PhysX IMU and PVA acceleration#6439
Conversation
Provide a shared standalone benchmark for the PhysX IMU and PVA update paths so baseline, cached-eager, and recorded-launch latency can be compared under the same workload.
Reuse stable typed PhysX buffers and recorded Warp commands to reduce per-update Python overhead. Refresh changing masks and timesteps on the recorded commands so runtime behavior remains unchanged.
Keep the IMU and PVA benchmark with the PhysX package sensor benchmarks so backend-specific performance tools share one discoverable location.
The recorded-launch unit tests allocate directly on cuda:0 and would error rather than skip on CPU-only runners. Gate the module on CUDA availability and tag it for the short isaacsim CI lane.
The cached typed views and the recorded launch assume the PhysX getters refresh pointer-stable buffers in place. Verify the pointers on every fetch so a re-backed buffer fails loudly instead of silently freezing the sensor data.
A recording failure only warns and falls back to eager launches, so nothing would notice the optimization silently dying. Assert the recorded command exists after CUDA integration runs, pass production- shaped ProxyArray gravity in the unit fixture, and assert the PhysX getters are still called on replay since they refresh the buffers.
Derive finite-difference acceleration from the elapsed time between sensor samples. This keeps lazy reads and nonzero update periods correct while preserving recorded Warp launches.
github-actions
Bot
added
bug
|
Newton cross-backend validation: using this PR worktree source explicitly on PYTHONPATH, the Newton IMU and PVA ten-step lazy free-fall acceleration tests both pass (2 passed). Newton is not affected by this bug because it consumes native accelerometer/body-acceleration state rather than numerically differentiating sensor samples. No Newton code changes are needed. |
Greptile SummaryThis PR fixes a 4× acceleration over-reporting bug in the PhysX IMU and PVA sensors: both sensors were computing finite-difference acceleration using the most recent physics
Confidence Score: 3/5The production sensor code (kernels, imu.py, pva.py) is correct and well-structured. The new test file ships with two concrete bugs that will prevent the tests from passing as written. The kernel fix and recorded-launch refactor in production code are sound, but source/isaaclab_physx/test/sensors/test_imu_pva_recorded_launch.py needs the most attention: missing Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _make_sensor(sensor_type: str, use_recorded_launch: bool = True): | ||
| """Create a two-environment IMU or PVA without a USD scene.""" | ||
| device = "cuda:0" | ||
| transforms_torch = torch.tensor( | ||
| [ | ||
| [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 1.0], | ||
| [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 1.0], | ||
| ], | ||
| dtype=torch.float32, | ||
| device=device, | ||
| ) | ||
| velocities_torch = torch.tensor( | ||
| [ | ||
| [1.0, 0.0, 0.0, 0.0, 0.0, 0.0], | ||
| [1.0, 0.0, 0.0, 0.0, 0.0, 0.0], | ||
| ], | ||
| dtype=torch.float32, | ||
| device=device, | ||
| ) | ||
| coms_torch = torch.tensor( | ||
| [ | ||
| [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 1.0], | ||
| [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 1.0], | ||
| ], | ||
| dtype=torch.float32, | ||
| ) | ||
| transforms = wp.from_torch(transforms_torch.contiguous()).view(wp.transformf) | ||
| velocities = wp.from_torch(velocities_torch.contiguous()).view(wp.spatial_vectorf) | ||
| coms = wp.from_torch(coms_torch.contiguous()).view(wp.transformf) | ||
| rigid_view = _FakeRigidView(transforms, velocities, coms) | ||
|
|
||
| sensor_cls = Imu if sensor_type == "imu" else Pva | ||
| sensor = sensor_cls.__new__(sensor_cls) | ||
| sensor.cfg = SimpleNamespace(prim_path=f"/World/{sensor_type.upper()}") | ||
| sensor._device = device | ||
| sensor._num_envs = 2 | ||
| sensor._view = rigid_view | ||
| sensor._dt = 0.5 | ||
| sensor._timestamp = wp.ones(2, dtype=wp.float32, device=device) | ||
| sensor._offset_pos_b = wp.zeros(2, dtype=wp.vec3f, device=device) | ||
| sensor._offset_quat_b = wp.array( | ||
| [wp.quatf(0.0, 0.0, 0.0, 1.0), wp.quatf(0.0, 0.0, 0.0, 1.0)], dtype=wp.quatf, device=device | ||
| ) | ||
| sensor._prev_lin_vel_w = wp.zeros(2, dtype=wp.vec3f, device=device) | ||
| sensor._coms_buffer = wp.zeros(2, dtype=wp.transformf, device=device) | ||
| sensor._raw_transforms = None | ||
| sensor._raw_velocities = None | ||
| sensor._raw_coms = None | ||
| sensor._update_cmd = None | ||
| sensor._update_env_mask = None | ||
| sensor._update_inv_dt = None | ||
| sensor._use_recorded_launch = use_recorded_launch | ||
| sensor._initialize_handle = None | ||
| sensor._invalidate_initialize_handle = None | ||
| sensor._prim_deletion_handle = None | ||
|
|
||
| if sensor_type == "imu": | ||
| sensor._data = ImuData() | ||
| sensor._data.create_buffers(num_envs=2, device=device) | ||
| sensor._gravity_bias_w = wp.zeros(2, dtype=wp.vec3f, device=device) | ||
| else: | ||
| sensor._data = PvaData() | ||
| sensor._data.create_buffers(num_envs=2, device=device) | ||
| sensor._prev_ang_vel_w = wp.zeros(2, dtype=wp.vec3f, device=device) | ||
| sensor.GRAVITY_VEC_W = ProxyArray(wp.zeros(2, dtype=wp.vec3f, device=device)) | ||
|
|
||
| env_mask = wp.ones(2, dtype=wp.bool, device=device) | ||
| return sensor, rigid_view, velocities_torch, env_mask |
There was a problem hiding this comment.
_timestamp_last_update causes AttributeError in all kernel-launching tests
_make_sensor creates sensors via __new__, bypassing __init__ and _setup_impl. sensor_base.py (line 258) initialises _timestamp_last_update = wp.zeros_like(self._timestamp) only through the normal constructor path — so the fake sensor never gets this attribute. Every test that eventually calls _launch_update passes self._timestamp_last_update to wp.launch, raising AttributeError and failing.
Beyond the crash, the expected values in test_sensor_records_and_replays_changed_runtime_inputs were calibrated for the old scalar inv_dt = 1.0 / self._dt path: sensor._dt = 0.5 → inv_dt = 2.0 → acc = 2.0. With the new kernel computing inv_dt from timestamp - timestamp_last_update, timestamp=1.0 and timestamp_last_update=0.0 (zeros) gives inv_dt = 1.0 and acc = 1.0, not 2.0. The sensor._dt = 0.25 assignment on line 175 is now a no-op that no longer changes the kernel's inv_dt.
_make_sensor should add sensor._timestamp_last_update at an appropriate value (e.g., wp.full(2, 0.5, dtype=wp.float32, device=device) to preserve inv_dt = 2.0 and keep the existing assertions) and the stale sensor._dt assignments should be either removed or replaced with _timestamp/_timestamp_last_update adjustments that actually control the computed inv_dt.
| def test_sensor_invalidation_drops_cached_launch_state(monkeypatch, sensor_type): | ||
| """Physics invalidation should release cached PhysX views and the recorded command.""" | ||
| sensor, _, _, _ = _make_sensor(sensor_type) | ||
| sensor._raw_transforms = object() | ||
| sensor._raw_velocities = object() | ||
| sensor._raw_coms = object() | ||
| sensor._update_cmd = object() | ||
| sensor._update_env_mask = object() | ||
| sensor._update_inv_dt = 1.0 | ||
| base_cls = BaseImu if sensor_type == "imu" else BasePva | ||
| monkeypatch.setattr(base_cls, "_invalidate_initialize_callback", lambda self, event: None) | ||
|
|
||
| sensor._invalidate_initialize_callback(None) | ||
|
|
||
| assert sensor._view is None | ||
| assert sensor._raw_transforms is None | ||
| assert sensor._raw_velocities is None | ||
| assert sensor._raw_coms is None | ||
| assert sensor._update_cmd is None | ||
| assert sensor._update_env_mask is None | ||
| assert sensor._update_inv_dt is None |
There was a problem hiding this comment.
_update_inv_dt is never cleared by _invalidate_initialize_callback, so the final assertion will always fail
The test sets sensor._update_inv_dt = 1.0 and then asserts sensor._update_inv_dt is None after _invalidate_initialize_callback runs. But neither Imu._invalidate_initialize_callback nor Pva._invalidate_initialize_callback touches this attribute — the implementations reset _raw_transforms, _raw_velocities, _raw_coms, _update_cmd, and _update_env_mask, but not _update_inv_dt. The attribute appears to be a leftover from an earlier design where inv_dt was stored as a mutable launch parameter; the final implementation dropped it in favour of kernel-side timestamp arithmetic.
Either add self._update_inv_dt = None to both _invalidate_initialize_callback implementations, or (if the attribute is truly dead) remove the _update_inv_dt assignments and assertion from the test entirely.
2 participants
Summary
This PR is stacked on #6390. Until #6390 merges, its commits are included in this comparison against develop.
Root cause
IMU and PVA cached velocity only when their buffers were recomputed, but divided the next velocity delta by the most recent physics-step dt. With four lazy physics updates or an update period of four physics steps, both sensors reported 4x acceleration.
The kernels now derive a per-environment interval from timestamp minus timestamp_last_update.
Validation
Performance
Benchmarking is deferred until the test machine is connected to AC power. The change removes scalar launch-parameter updates and retains the recorded-launch path; this PR makes no performance claims.